-
Notifications
You must be signed in to change notification settings - Fork 4.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
solution #4961
base: master
Are you sure you want to change the base?
solution #4961
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job so far! 😄 However, there are a few areas that need improvement. Please use semantic tags instead of div
for the stars
blocks, and ensure the path to the images is consistent. Also, remember to add empty lines between multiline sibling blocks of HTML for readability. Don't forget to include a description for the content of your webpage using the <meta name="description" content="...">
tag in your <head>
section. Keep up the good work! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -12,7 +13,49 @@ | |||
href="./style.css" | |||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTML Formatting: The attributes of your link
tag are not correctly formatted. Each attribute should start on a new line with a 2-space indentation relative to the tag.
src/index.html
Outdated
<div class="stars stars--0"> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTML Formatting: Add empty lines between multiline sibling blocks of HTML to improve readability. In this case, you should add an empty line between each div
with the class stars
.
src/index.html
Outdated
<div class="stars stars--1"> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTML Formatting: Add empty lines between multiline sibling blocks of HTML to improve readability. In this case, you should add an empty line between each div
with the class stars
.
src/index.html
Outdated
<div class="stars stars--2"> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTML Formatting: Add empty lines between multiline sibling blocks of HTML to improve readability. In this case, you should add an empty line between each div
with the class stars
.
src/index.html
Outdated
<div class="stars stars--3"> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTML Formatting: Add empty lines between multiline sibling blocks of HTML to improve readability. In this case, you should add an empty line between each div
with the class stars
.
src/index.html
Outdated
<div class="stars stars--5"> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTML Formatting: Add empty lines between multiline sibling blocks of HTML to improve readability. In this case, you should add an empty line between each div
with the class stars
.
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
<div class="stars__star"></div> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTML Content: It would be better to use semantic tags instead of div
for the stars
blocks. Consider using a semantic tag like section
or article
that would better describe the content.
src/style.css
Outdated
* { | ||
margin: 0; | ||
padding: 0; | ||
box-sizing: border-box; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using the *
selector as it impacts performance. It's better to set styles only for elements that require them. You can find this in the task checklist under the CSS section.
.stars--1 :first-child { | ||
background-image: url(./images/star-active.svg); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path to the images is not consistent. Here you're using ./images/star-active.svg
, while in other places you're using images/star-active.svg
. It's not an error, but it's better to keep the code consistent.
.stars--5 :nth-child(-n + 1), | ||
.stars--5 :nth-child(-n + 2), | ||
.stars--5 :nth-child(-n + 3), | ||
.stars--5 :nth-child(-n + 4), | ||
.stars--5 :nth-child(-n + 5) { | ||
background-image: url(images/star-active.svg); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simplify the CSS rule for .stars--5
by using :nth-child(-n + 5)
only. There's no need to repeat it for each child.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on your code! 👍 However, there are a few areas that need some improvement. Please consider using a fallback font, avoid fixing container size, be consistent with your margins, and avoid using the *
selector for styling. Also, ensure the indentation for the attributes is consistent across the document and consider using a templating system or a loop to avoid code repetition. Keep up the good work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
<div class="stars stars--0"> | ||
<div class="stars__star"> | ||
</div> | ||
|
||
<div class="stars__star"> | ||
</div> | ||
|
||
<div class="stars__star"> | ||
</div> | ||
|
||
<div class="stars__star"> | ||
</div> | ||
|
||
<div class="stars__star"> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block of code is repeated multiple times with only minor changes. Consider using a templating system or a loop in a scripting language to generate these elements programmatically. This will make your code more maintainable and less prone to errors.
margin: 0; | ||
padding: 0; | ||
box-sizing: border-box; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using the *
selector for styling. It impacts performance as it affects all elements. Try to set styles only for elements that require them.
margin: 0; | ||
padding: 0; | ||
box-sizing: border-box; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a checklist item that suggests not to use tag names for styling, except for html
and body
.
border-radius: 0.5px; | ||
background-repeat: no-repeat; | ||
background-position: center; | ||
margin-right: 4px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember to use fallback fonts. It's an alternative font-family in case the main one doesn't work. You can specify a list of fonts separated by commas. The browser will use the first font in the list available on the user's computer.
background-repeat: no-repeat; | ||
background-position: center; | ||
margin-right: 4px; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be consistent with your margins. It seems you are applying the margin-right
property to all .stars__star
elements. This could potentially lead to margin collapse. Consider applying margins only to the top or bottom, but not both.
.stars--5 :nth-child(-n + 4), | ||
.stars--5 :nth-child(-n + 5) { | ||
background-image: url(images/star-active.svg); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good practice to avoid fixing container size if there is no such a requirement. Let the content size dictate it. This can help to avoid overflow or accidental scroll bar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on the HTML and CSS! Your code is well-structured and readable. There are a few minor issues that need to be addressed, including the use of the *
selector and the placement of empty lines in your HTML. These aren't major issues, but they are worth noting for future projects. Keep up the good work and continue to refine your coding skills! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
<div class="stars__star"></div> | ||
|
||
<div class="stars__star"></div> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the checklist, you should add empty lines between multiline sibling blocks of HTML to simplify reading. However, you have added empty lines between child elements, which is not recommended. You should remove the empty lines between the div
elements with the class stars__star
.
body { | ||
margin: 0; | ||
padding: 0; | ||
box-sizing: border-box; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the checklist, you should avoid using the *
selector for styling, as it impacts performance. Although this is a minor issue, it's a good practice to style only the elements that require it.
.stars--5 :nth-child(-n + 1), | ||
.stars--5 :nth-child(-n + 2), | ||
.stars--5 :nth-child(-n + 3), | ||
.stars--5 :nth-child(-n + 4), | ||
.stars--5 :nth-child(-n + 5) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a mistake in your CSS selectors. The :nth-child(-n + 1)
selector in the .stars--5
rule will only select the first child, but you probably intended to select all children. To select all children, you can simply use the :nth-child(n)
selector. Also, there's no need to repeat the same selector for each child. You can combine them into a single rule: .stars--5 :nth-child(n)
.
Typical Mistakes
fromBEM
lesson theory are checked.